test: regression test all files #325
Conversation
aner-starkware
changed the base branch from
main
to
aner/refactor_download_directory
aner-starkware
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #325 +/- ##
=======================================
Coverage 70.03% 70.03%
=======================================
Files 38 38
Lines 2109 2109
Branches 2109 2109
=======================================
Hits 1477 1477
Misses 562 562
Partials 70 70 ☔ View full report in Codecov by Sentry. |
|
Benchmark movements: |
aner-starkware
force-pushed
the
aner/regression_test_all_files
branch
from
304f54d
to
7704481
Compare
aner-starkware
requested review from
TzahiTaub,
dorimedini-starkware and
nimrod-starkware
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @aner-starkware, @dorimedini-starkware, and @nimrod-starkware)
crates/committer_cli/src/tests/regression_tests.rs line 139 at r2 (raw file):
} pub async fn assert_committer_flow(input: &str, output_path: &str) {
Personal preference
Suggestion:
test_single_committer_flow(crates/committer_cli/src/tests/regression_tests.rs line 171 at r2 (raw file):
assert!(execution_time.as_secs_f64() < MAX_TIME_FOR_COMMITTER_FLOW_BECHMARK_TEST); } #[ignore = "To avoid running the benchmark test in Coverage or without the --release flag."]
Please change the benches directory to something more general (like flow_test_inputs or test_inputs) to match both the regression tests (which uses most of these files) and the becnhmarking. The current name is misleading.
Suggestion:
regression crates/committer_cli/src/tests/regression_tests.rs line 177 at r2 (raw file):
} #[ignore = "To avoid running the benchmark test in Coverage or without the --release flag."]
Suggestion:
regressioncrates/committer_cli/src/tests/regression_tests.rs line 192 at r2 (raw file):
} assert_eq!(file_counter, EXPECTED_NUMBER_OF_FILES); }
Please start with this check, it's a shame to run the committer over 99 files to fail in this. You can copy the iterator and call count on one of the copies at the beginning. Maybe there is a more efficient method.
Code quote:
assert_eq!(file_counter, EXPECTED_NUMBER_OF_FILES);
}
aner-starkware
force-pushed
the
aner/regression_test_all_files
branch
from
7704481
to
9bb981a
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware, @nimrod-starkware, and @TzahiTaub)
crates/committer_cli/src/tests/regression_tests.rs line 171 at r2 (raw file):
Previously, TzahiTaub (Tzahi) wrote…
Please change the
benchesdirectory to something more general (like flow_test_inputs or test_inputs) to match both the regression tests (which uses most of these files) and the becnhmarking. The current name is misleading.
Done.
crates/committer_cli/src/tests/regression_tests.rs line 177 at r2 (raw file):
} #[ignore = "To avoid running the benchmark test in Coverage or without the --release flag."]
Done.
crates/committer_cli/src/tests/regression_tests.rs line 192 at r2 (raw file):
Previously, TzahiTaub (Tzahi) wrote…
Please start with this check, it's a shame to run the committer over 99 files to fail in this. You can copy the iterator and call
counton one of the copies at the beginning. Maybe there is a more efficient method.
Done. Though I'm not sure that I agree.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @dorimedini-starkware and @nimrod-starkware)
crates/committer_cli/src/tests/regression_tests.rs line 192 at r2 (raw file):
Previously, aner-starkware wrote…
Done. Though I'm not sure that I agree.
About what?
* test: regression test for all regression files * refactor: extract assert committer flow function
This change is